Skip to content

Conversation

@abrahamwolk
Copy link
Collaborator

@abrahamwolk abrahamwolk commented Apr 23, 2024

This PR implements part of the functionality described in #2972. In particular, it implements points 1 and 2 described in that discussion, i.e.:

  1. Extended support for copying PV names by right-clicking in the Display Runtime
  2. Support for copying PV names in the Widget Info window

The description of 2 in the linked discussion is not exactly what has been implemented: checkboxes have been added to the Widget Info dialog (and it is possible to right-click and select "Select All" and "De-Select All"), and only one button "Copy..." has been added. The "Copy..." button opens a dialog, in which one then copy the selected PV names, either with or without descriptions.

I am working on a PR for point 3 in the linked discussion also. (I.e., 3. Modified and extended functionality to paste several PV names (with an optional display name) into the Data Browser.)
EDIT: The PR #3000 implements point 3 in the linked discussion also. (I.e., 3. Modified and extended functionality to paste several PV names (with an optional display name) into the Data Browser.)

I would be very happy to discuss the functionality, the implementation, or suggestions for improvements.

Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Occurrences of "\n" should maybe be replaced by System.lineSeparator().

In the widget info dialog I'd prefer "select all" and "deselect all" to be handled by a checkbox in the Selected table header instead of a context menu. This would be more in line with common practice.

I would also suggest another button "Copy PV names" in the widget info dialog as I assume this would be the most common use case. It would save the user from interacting with another dialog.

In the Copy PV Name(s) dialog I find the repeated use of plural (s) a bit disturbing. Since upon launch of the dialog the PV selection is known, one could define separate strings for singular and plural.

@abrahamwolk
Copy link
Collaborator Author

Occurrences of "\n" should maybe be replaced by System.lineSeparator().

In the widget info dialog I'd prefer "select all" and "deselect all" to be handled by a checkbox in the Selected table header instead of a context menu. This would be more in line with common practice.

I would also suggest another button "Copy PV names" in the widget info dialog as I assume this would be the most common use case. It would save the user from interacting with another dialog.

In the Copy PV Name(s) dialog I find the repeated use of plural (s) a bit disturbing. Since upon launch of the dialog the PV selection is known, one could define separate strings for singular and plural.

Thanks, these are all nice improvements I think! I will implement these.

@abrahamwolk
Copy link
Collaborator Author

@georgweiss I have implemented the suggestions you made. I left the context-menu for selecting and de-selecting all elements intact, but the header of the table column is now a checkbox that can be used to toggle the selection of all or no PV names.

The PR is ready for review again.

@abrahamwolk abrahamwolk requested a review from georgweiss May 8, 2024 12:39
…V names to the clipboard from 'org.phoebus.applications.probe' to 'org.csstudio.display.builder.runtime'.
@abrahamwolk
Copy link
Collaborator Author

@kasemir , @shroffk : Regarding this PR and the PR #2972: Would you mind answering my idea in #3057 (comment)?

@abrahamwolk
Copy link
Collaborator Author

I am closing this PR since the functionality (1) has partly been implemented in an alternative way in #3057 and (2) there is no consensus for adding the functionality to append display names/descriptions to copied PV names.

@abrahamwolk abrahamwolk deleted the CSSTUDIO-2072-contextMenu branch July 31, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants